Possible overflow issue on expressions. #101
Conversation
|
Well you are just making the number unsigned which means that now negative expressions don't work anymore, with the tradeoff of doubling the positive number range... I don't think this is a useful change (negative expressions are more popular). It would be better to detect and fix: :-) |
|
Maybe I wasn't clear. The concern isn't actually on overflow, but on that signed overflow is undefined behavior. According to this page, the standard states unsigned overflow wraps around, and is portable, unless strong optimizations are set. However, I'm afraid the operation if (n & ~(~(unsigned tcsh_number_t) 0 >> 1))isn't portable, since it's bitwise. I'm (very likely wrongly) assuming the last bit is the sign bit on every machine. I don't know what the standard states on bitwise operations and sign bit. I had a problem with relational operators, but switch ((int) i) {
tcsh_number_t is;
case GTR:
if ((i = egetn(p1)) &
~(~(unsigned tcsh_number_t) 0 >> 1))
is = -(tcsh_number_t) -i;
else
is = i;
if ((i = egetn(p2)) &
~(~(unsigned tcsh_number_t) 0 >> 1))
i = is > -(tcsh_number_t) -i;
else
i = is > (tcsh_number_t) i;
break;
case GTR | 1:
if ((i = egetn(p1)) &
~(~(unsigned tcsh_number_t) 0 >> 1))
is = -(tcsh_number_t) -i;
else
is = i;
if ((i = egetn(p2)) &
~(~(unsigned tcsh_number_t) 0 >> 1))
i = is >= -(tcsh_number_t) -i;
else
i = is >= (tcsh_number_t) i;
break;
case LSS:
if ((i = egetn(p1)) &
~(~(unsigned tcsh_number_t) 0 >> 1))
is = -(tcsh_number_t) -i;
else
is = i;
if ((i = egetn(p2)) &
~(~(unsigned tcsh_number_t) 0 >> 1))
i = is < -(tcsh_number_t) -i;
else
i = is < (tcsh_number_t) i;
break;
case LSS | 1:
if ((i = egetn(p1)) &
~(~(unsigned tcsh_number_t) 0 >> 1))
is = -(tcsh_number_t) -i;
else
is = i;
if ((i = egetn(p2)) &
~(~(unsigned tcsh_number_t) 0 >> 1))
i = is <= -(tcsh_number_t) -i;
else
i = is <= (tcsh_number_t) i;
break;
}made the fix. Note I cast operands that can be represented in signed quantity on comparison because, from what I've read, the standard states comparison between signed and unsigned, before the comparison, the signed operand is converted to unsigned. I had problems without a cast. Negative expressions are working fine for me, though I've tested only on one machine. |
I noticed
putn1breaks the conversion if the last bit is set.On my machine (an ARM-based cellphone)
raises a
@: Badly formed number.error. I inspected why's that, and turns out the conversion yields a((left parenthesis) character.I can tell the program isn't careful on overflow, since none of the variables and functions are typed
unsigned, and there doesn't seem to exist any validity checks on overflow.Typing
putn1's argument asunsignedremedied the issue.I'm not sure if my solution is portable, but is what made the fix. I also provide portability among different character encodings.
Lastly, line 193 on sh.exp.c has a wrong type. On my machine
yields
0.